-
Notifications
You must be signed in to change notification settings - Fork 1
feat: add support for dynamic theme switching #140
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughAdds a new public enum DynamicTheme implementing runtime theme selection (LUMO, AURA, BASE) with initialization, prepare, and apply lifecycle methods; integrates a theme selector into TabbedDemo; adjusts footer/select styles; and updates AppShell configuration to initialize the dynamic theme when supported. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/main/java/com/flowingcode/vaadin/addons/demo/DynamicTheme.java`:
- Around line 70-73: The calls to VaadinSession.getCurrent() in
isFeatureInitialized(), getCurrent(), and initialize() can be null and cause
NPEs; update each method to first retrieve VaadinSession session =
VaadinSession.getCurrent() and guard against null (e.g., return false from
isFeatureInitialized(), return null or throw a clear IllegalStateException from
getCurrent(), and no-op or delay initialization in initialize()) before
accessing session.getAttribute(...) or session.setAttribute(...), preserving
existing behavior when session is present and adding a clear, defensive early
return when it's absent.
- Around line 166-178: The injected JS in component.getElement().executeJs calls
document.startViewTransition and then accesses properties on link without
verifying link exists, which can throw in non-Chromium browsers or when the
stylesheet isn't present; update the script to first check that
document.startViewTransition is a function and fall back to executing the
transition callback directly if unavailable, and add a null check for link
(e.g., if (link) { ... }) before reading or writing link.rel or link.disabled
when iterating over the href list (note the passed href parameter $0), so the
code safely handles missing <link> elements and browsers without
startViewTransition support.
🧹 Nitpick comments (2)
src/main/java/com/flowingcode/vaadin/addons/demo/DynamicTheme.java (1)
107-116: Consider usingtheme.getHref()instead of hardcoded paths.The enum already defines
hreffor each theme. Usingtheme.getHref()would be more consistent and DRY.♻️ Proposed refactor
- switch (theme) { - case AURA: - settings.addLink(Position.APPEND, "stylesheet", "aura/aura.css"); - break; - case LUMO: - settings.addLink(Position.APPEND, "stylesheet", "lumo/lumo.css"); - break; - default: - break; + if (theme.getHref() != null) { + settings.addLink(Position.APPEND, "stylesheet", theme.getHref()); }src/main/java/com/flowingcode/vaadin/addons/demo/TabbedDemo.java (1)
132-142: Consider null-safety for value change handler.While
Selecttypically won't fire change events with null values when properly configured, adding a defensive check prevents potential NPEs.🛡️ Proposed defensive check
- themeSelect.addValueChangeListener(ev -> ev.getValue().apply(this)); + themeSelect.addValueChangeListener(ev -> { + DynamicTheme theme = ev.getValue(); + if (theme != null) { + theme.apply(this); + } + });
2101ca1 to
53418c7
Compare
|



Summary by CodeRabbit
New Features
Style
Tests